feat: support for oidc max age#949
Conversation
dosubot
Bot
added
the
size:L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe OIDC authorize endpoint now parses and enforces ChangesOIDC prompt/max_age enforcement and auth_time wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/oidc_service.go (1)
938-949:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
max_ageis not extracted from JWT claims.
DecodeAuthorizeJWTextractspromptbut notmax_age. If a client sends the authorize request as a JWT (request object), themax_ageclaim will be ignored whilereq.MaxAgeremains empty.🔧 Proposed fix
return &AuthorizeRequest{ Scope: get("scope"), ResponseType: get("response_type"), ClientID: get("client_id"), RedirectURI: get("redirect_uri"), State: get("state"), Nonce: get("nonce"), CodeChallenge: get("code_challenge"), CodeChallengeMethod: get("code_challenge_method"), Prompt: get("prompt"), + MaxAge: get("max_age"), }, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/oidc_service.go` around lines 938 - 949, The DecodeAuthorizeJWT function is missing extraction of the max_age claim from the JWT when constructing the AuthorizeRequest struct. Add a new field MaxAge to the AuthorizeRequest return statement and populate it using the get() method with the parameter "max_age", following the same pattern used for other JWT claims like prompt, state, and nonce.
🧹 Nitpick comments (1)
internal/service/oidc_service.go (1)
676-678: ⚡ Quick winConsider OIDC spec compliance for
auth_timeduring refresh.Setting
auth_timeto0emits"auth_time": 0in the ID token (Unix epoch). Per OIDC Core spec,auth_timeshould reflect the original authentication time. Consider either:
- Persisting
auth_timein the OIDC session DB and retrieving it during refresh- Omitting
auth_timeentirely from refreshed tokens (useomitemptybehavior)The current approach could confuse relying parties that validate
auth_time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/oidc_service.go` around lines 676 - 678, The generateIDToken call in the refresh token flow is passing a hardcoded auth_time value of 0, which violates OIDC spec compliance. Either persist the original auth_time value when the OIDC session is first created and stored in the entry object, then retrieve and pass that value during refresh instead of 0, OR modify the generateIDToken function and the underlying token generation logic to support omitting auth_time entirely from the JWT when it's not available (using omitempty struct tags). This ensures that refreshed ID tokens either contain the original authentication time or exclude the field entirely rather than emitting a zero Unix timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/oidc_controller.go`:
- Around line 222-242: The code accesses userContext.Authenticated and
userContext.AuthTime in the max_age validation block without checking if
userContext is nil or in a valid state. If NewFromGin returns an error other
than ErrUserContextNotFound, userContext could be nil or partially initialized,
leading to a nil dereference. Add a nil check for userContext before the
condition that checks userContext.Authenticated to ensure the userContext is
properly initialized before accessing its fields.
---
Outside diff comments:
In `@internal/service/oidc_service.go`:
- Around line 938-949: The DecodeAuthorizeJWT function is missing extraction of
the max_age claim from the JWT when constructing the AuthorizeRequest struct.
Add a new field MaxAge to the AuthorizeRequest return statement and populate it
using the get() method with the parameter "max_age", following the same pattern
used for other JWT claims like prompt, state, and nonce.
---
Nitpick comments:
In `@internal/service/oidc_service.go`:
- Around line 676-678: The generateIDToken call in the refresh token flow is
passing a hardcoded auth_time value of 0, which violates OIDC spec compliance.
Either persist the original auth_time value when the OIDC session is first
created and stored in the entry object, then retrieve and pass that value during
refresh instead of 0, OR modify the generateIDToken function and the underlying
token generation logic to support omitting auth_time entirely from the JWT when
it's not available (using omitempty struct tags). This ensures that refreshed ID
tokens either contain the original authentication time or exclude the field
entirely rather than emitting a zero Unix timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 30182146-d821-4934-bb60-ff56143d5be2
📒 Files selected for processing (6)
frontend/src/lib/hooks/screen-params.tsfrontend/src/pages/authorize-page.tsxfrontend/src/pages/login-page.tsxinternal/controller/oidc_controller.gointernal/model/context.gointernal/service/oidc_service.go
|
This one LGTM too, just ping me when #948 is merged and I'll sign off. |
dosubot
Bot
added
size:S
2 participants
Depends on #948
Summary by CodeRabbit
Release Notes
New Features
promptparameter support and validation of parameter combinations.max_ageparameter to enforce maximum session age requirements.Bug Fixes